feat(code): add options "create-bytes" and "update-bytes" #123
Conversation
hasezoey
left a comment
hasezoey
left a comment
There was a problem hiding this comment.
mostly looks good, though there are some points:
- the
main.rsstruct comments will need to be updated (are still copy-paste from the string version) - the lifetimes are not accounted for yet (only string lifetimes, see comment on
code.rs) - also consider adding a test for a lifetime string & lifetime bytes test combined (where both are set to generate lifetimes like both being
coworslice)
also dont just post a PR without a description, provide a simple explanation of what this PR does and why it may be necessary (instead of just using Vec<[u8]> everywhere)
|
I should have fixed up everything at this point; I apologize for the half-assed nature of the initial PR. Looking at the code now, I'm convinced that there is a compelling argument to merge the two into a single flag; I struggle to think of a situation where the user wants to avoid moves/allocations for Strings but not for Vec. This would also avoid the need for testing that the lifetime string is correct in the generated struct when both are present. |
hasezoey
left a comment
hasezoey
left a comment
There was a problem hiding this comment.
This PR looks good now, thanks for the work (i at least dont see any issues, but the test output is currently not compile-tested, see #114)
I'm convinced that there is a compelling argument to merge the two into a single flag; I struggle to think of a situation where the user wants to avoid moves/allocations for Strings but not for Vec.
i guess you could say that, but this is for the future and if changed, then likely will only be changed for the main.rs configuration (because why change the library one, if it already exists and is not much burden and so has more configurability?)
Thanks. It looks like the compile-testing is a WIP. I can revisit adding that onto this once I have a merged example to work with.
My main motivation would be to simplify the library code; there is something to be said for configurability being meaningless if it's not actually put to use. Having said that, I've already scratched my itch. I also have a follow-up commit documenting the flags. |
2 participants
Add feature flags for reference- and cow- types for byte blobs in update/create akin to the feature flags for string types.
This allows one to avoid additional allocations or moves in Create and Update structs when dealing with byte blobs similar to allocations avoided by using &str and Cow with strings.